-
-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[internal] BSP: switch to aggregate build targets #14835
Conversation
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
Code indexing for first-party sources works with this PR. Consider this PR an intermediate step though, there is lots of refactoring to be done and probably changes to DX for follow-ons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, the dict option makes sense to me.
What would make sense for testing this? Some of the targets.py stuff looks intricate
@@ -66,6 +66,15 @@ def activated(cls, union_membership: UnionMembership) -> bool: | |||
advanced=True, | |||
) | |||
|
|||
target_mapping = DictOption[Any]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be DictOption[List[str]]
?
# Naive algorithm (since we only support Java and Scala backends), find the metadata request type that cannot | ||
# merge from another and put that first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really follow what this code is doing. Sounds like this will be generalized in some future? I was going to ask a unit test but probably not worth it here?
field_sets_by_lang_id[metadata_request_type.language_id].add( | ||
field_set_type.create(tgt) | ||
) | ||
# lang_ids_by_field_set[field_set_type].add(metadata_request_type.language_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale?
# TODO: Consider how to check whether the provided languages are compatible or whether compatible resolves | ||
# selected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd use the CoursierKey
thing. It iterates over the targets to make sure their resolve is correct. You may want to enrich the error message to say how users should fix it. Relates to #14738
# Pretend to merge the metadata into a single piece of metadata, but really just choose the metadata | ||
# from the last provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come?
# base_dir = build_root.pathlib_path | ||
# if merged_sources_dirs: | ||
# _logger.info(f"merged_sources_dirs = {merged_sources_dirs}") | ||
# common_path = os.path.commonpath(list(merged_sources_dirs)) | ||
# _logger.info(f"common_path = {common_path}") | ||
# if common_path: | ||
# base_dir = base_dir.joinpath(common_path) | ||
# _logger.info(f"base_dir = {base_dir}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale?
elif len(source_root_paths) == 1: | ||
base_dir = build_root.pathlib_path.joinpath(list(source_root_paths)[0]) | ||
else: | ||
raise ValueError("Multiple source roots not supported for BSP build target.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably helpful to say what the target is from the dict option, along with the source roots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm the only reader for this exception currently. Will improve messages once we decide on a final UX which won't happen until I get third-party indexing working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also because we probably will support multiple source roots but that can be explored in a follow-on PR.
# classpath=( | ||
# build_root.pathlib_path.joinpath( | ||
# f".pants.d/bsp/jvm/resolves/{resolve.name}/lib/{output_file}" | ||
# ).as_uri(), | ||
# ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be deleted?
# assert len(coarsened_targets) == 1 | ||
# coarsened_target = coarsened_targets[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be deleted?
# assert len(coarsened_targets) == 1 | ||
# coarsened_target = coarsened_targets[0] | ||
resolve = await Get(CoursierResolveKey, CoarsenedTargets, coarsened_targets) | ||
# output_file = compute_output_jar_filename(coarsened_target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be deleted?
Not yet since this PR provides a "bare minimum" UX for development, and may not be the final UX that users see. I will add such tests once we have a basic prototype working and can iterate on UX. As mentioned in an earlier comment, consider this PR as merely an intermediate point which gets first-party code indexing prototyped, but is still under active development for third-party dep code indexing. The reason I broke it out as PR now was because the change was getting large enough that it warranted breaking out now before I work on third-party code indexing. |
I'll clean up the commented-out code, but for now I would like to land this PR pretty much as-is. We don't expose the BSP support to users, so fine in my view to clean up once I get a prototype working. Also, @stuhood and I will be chatting about UX next week. |
[ci skip-rust] [ci skip-build-wheels]
Switch BSP core rules to allow configuration of a small number of BSP build targets that aggregate multiple Pants targets. The base directory for a BSP build target is computed to be the source root for the targets contained within.
Note: